atelet: node-local file cache library (filecache, M1) by dberkov · Pull Request #1517 · agent-substrate/substrate

Dmitry Berkovich (dberkov) · 2026-09-05T02:46:11Z

Implements milestone M1 of the node-local artifact cache proposed in #690 (design in the issue comment): a generic cmd/atelet/internal/filecache package that will back golden-snapshot restores (today re-downloaded per actor on every start/resume) and later the sandbox-asset fetches. it reads well commit by commit:

atelet: add filecache store skeleton — constructor-only Keys (SHA256Key content-addressed, URIKey for immutable sources; prefix-disjoint canonical forms, entry dir = sha256(key)), the entries/ + tmp/ + .rm-* layout, SweepDebris (startup crash-debris reaper), TotalBytes (GC budget measure), debug-only meta.json.
atelet: add filecache singleflight retrieval (GetFileTo) — atomic get-and-link: per-key singleflight on context.WithoutCancel + fetch timeout (a canceled caller never aborts the download others wait on; no negative caching); fetch into tmp/, validate, chmod 0444 (in-place writes fail loudly instead of poisoning shared bytes), publish by one atomic rename; hit = hard link + LRU touch under hitMu.RLock. Path-based FileFetcher so ategcs's sparse zstd download plugs in unchanged; %w wrapping end-to-end for ateerrors classification.
atelet: move the sparse file copy helpers into internal/sparsefile — mechanical move of copyFile/copySparse/kernelCopyRange (and their tests) out of package main so filecache can reuse them; adds Copy(src, dst *os.File) for caller-owned handles (source opened before its name can vanish, destination created O_EXCL).
filecache: add GetFileCopyTo for consumers that mutate staged files — the second serving mode: a private, hole-preserving copy (mode 0600) instead of a read-only hard link, for consumers that rewrite staged files in place (ateom-microvm rewrites config.json at restore and merges deltas into memory-ranges at suspend — a shared inode would be corrupted). The copy reads a handle opened under the hit lock, so an eviction racing the copy retires only the entry's name; a copy needs no same-mount constraint.
atelet: add filecache eviction (EvictUnused) — pressure-driven only: min-age gate, unlinked-first then LRU ordering, stop at target. Two-phase retire inside the key's singleflight + hitMu exclusive (moved last-use clock or in-flight fetch vetoes; rename to .rm-*), slow RemoveAll after all retires outside the hot-path locks. Stats distinguish Retired (namespace removal, irreversible at rename) from FreedBytes (credited only after physical removal succeeds) and PendingBytes (retired but consumer-linked; kernel frees later). Copied-out entries carry no links, so eviction is free to take them — existing copies are private inodes and unaffected.
atelet: document filecache contracts and stress the get/evict races — package-doc contracts (link-out immunity, copy-out privacy, min-age sizing rule, read-only shared bytes, key immutability) plus a race-detector stress test: concurrent getters and evictors on shared keys; every get must succeed with intact content.

The core safety property throughout: eviction can only ever cost a refetch — never break a consumer. Hard-linked files are protected by the link itself (the consumer's inode survives eviction); copies are private inodes; the min age covers the publish-to-use window.

Follow-ups per the design: M2 wires a golden store into Restore (downloadExternalCheckpoint/downloadCombinedCheckpoint) with a GC driver loop — gVisor restores get hard links, micro-VM restores get copies; M3 adds GetFile/GetDir + the sandbox-record root set and migrates fetchAsset/fetchGVisorRelease.

Tested: go test -race -count=3 ./cmd/atelet/internal/filecache/; every commit builds and passes tests individually; golangci-lint, gofmt, and boilerplate checks clean.

🤖 Generated with Claude Code

Eitan Yarmush (EItanya)

Superseded by the comment-only review. The change request has been dismissed.

Eitan Yarmush (EItanya)

🤖 AI-generated review. I looked through them to make sure they weren't crazy and they all seem worth looking at.

Use allocated bytes for sparse-file accounting — evict.go:213
sizeEntry and TotalBytes count logical file lengths. In a local reproduction, an entry whose regular files occupied 8 KiB was reported as freeing over 1 GiB. This can trigger unnecessary eviction and stop reclamation before the requested disk space is freed. Please use allocated bytes consistently for budgeting and eviction, such as Stat_t.Blocks * 512, and add a sparse-file accounting test.
Synchronize the failed-fetch test deterministically — getfileto_test.go:197
started.Done() runs before callers enter GetFileTo, so the fetch can finish before everyone joins. Late callers correctly retry, violating the test’s one-fetch assertion. A focused race-enabled run failed 16 times out of 100. Please wait until the callers are actually waiting on the flight before releasing the fetch, using testing/synctest or an equivalent deterministic barrier.
Report filesystem errors during eviction enumeration — evict.go:180
The Stat and sizeEntry error branches assume an entry disappeared during enumeration, swallowing permission and I/O errors too. An unreadable entry reproduced a nil error and entirely zero eviction statistics. This makes broken cache access indistinguishable from having no eligible entries. Please ignore only fs.ErrNotExist and return other errors with the affected path.
Clarify the copy helper’s destination requirements — sparsefile.go:78
Copy accepts open handles without requiring an empty destination. Copying an all-hole source over an existing file returns success while retaining the old destination bytes; this was reproduced locally. The current caller creates an empty file and is safe. The smallest fix is to document the empty, zero-offset destination requirement and add a contract check, or explicitly support overwriting.
Make eviction-race coverage deterministic — stress_test.go:113
The stress test discards eviction statistics and exercises only hard-link retrieval. One passing run performed just the eight initial fetches. Please add deterministic overlap tests for eviction versus both serving modes and verify that eviction actually occurred. Fetch-timeout behavior also needs a test beyond checking the configured value.
Move test-only metadata reading out of production code — filecache.go:197
readEntryMeta is used only by tests. It can move into the test file, or the test can decode the metadata directly.

Validation: scoped golangci-lint and go vet passed. Atelet tests passed, and filecache/sparsefile passed three race-enabled runs. Focused repetitions reproduced the flaky test; isolated probes reproduced the accounting, filesystem-error, and existing-destination issues. Full repository verification and infrastructure E2E were not run.

Test cases, reproduction commands, and observed results.

Replaced with a comment-only review at the reviewer’s request: #1517 (review)

Dmitry Berkovich (dberkov) · 2026-09-09T01:00:08Z

Thanks — all six findings were real. All are fixed and folded into the commits that introduced them (so the branch still reviews commit-by-commit); force-pushed.

Allocated bytes — TotalBytes, sizeEntry, and every eviction figure (FreedBytes, PendingBytes, victim selection) now use a shared allocatedSize helper (Stat_t.Blocks * 512). This one mattered most here: the cache's flagship artifact is a sparse guest memory image, and logical sizes would have made the GC loop evict everything while freeing nothing. New tests pin the unit with a 64 MiB-logical / 4 KiB-data entry on both the sizing and eviction sides (skipping on filesystems that cannot report holes).
Flaky failed-fetch test — rewritten under testing/synctest: synctest.Wait() releases the failing fetch only once every caller is durably parked on the shared flight, which is exactly the observation the old started.Done() barrier could not make. 30 consecutive -race runs pass.
Eviction enumeration errors — listCandidates now skips only fs.ErrNotExist; permission/I/O failures are reported per-entry in the joined error while the healthy entries still get processed, so a broken cache is no longer indistinguishable from an empty one. Test: unreadable entry → error naming it, sibling still evicted.
sparsefile.Copy destination contract — documented and enforced: a non-empty destination is rejected up front (holes are never written, so stale bytes would show through). CopyFile is unaffected (os.Create truncates).
Stress determinism — the storm now runs both serving modes (half the getters use GetFileCopyTo), accumulates eviction stats, and fails if it retired nothing or never forced a refetch — a quiet run can no longer pass vacuously. Added a real fetch-timeout test (hung fetcher under synctest: deadline error, no debris, key recovers). We kept the dedicated retireEntry veto tests as the deterministic overlap coverage rather than adding a second orchestrated-overlap harness.
readEntryMeta — moved to the test file.

Eitan Yarmush (EItanya)

All comments addressed properly, and overall LGTM, does anyone else need to review this or should I do another final round?

Dmitry Berkovich (dberkov) · 2026-09-09T23:39:25Z

Let me rebase it first, so you will be able to merge it.

Introduce cmd/atelet/internal/filecache, the foundation of a node-local artifact cache: opaque entry keys (content-addressed sha256 and immutable-URI forms), the entries/tmp on-disk layout, a startup sweep for crash debris (unfinished fetches, interrupted evictions), and byte accounting for a GC budget. Golden snapshot restores download their files per actor with no reuse, and sandbox-asset fetches race concurrent downloads of the same asset; this package is the shared cache that will back both paths. Retrieval (singleflight fetch, atomic publication, hardlink-out) and eviction build on this skeleton in follow-up changes.

GetFileTo materializes a cached artifact at a destination path via hard link, fetching it on a miss. Concurrent callers for one key share a single fetch (singleflight), and the fetch runs detached from the callers' contexts bounded by the store's fetch timeout, so one canceled caller never aborts a download other callers are waiting on. There is no negative caching: a failed fetch reaches every waiting caller and the next call starts fresh. A fetch lands in tmp/, must produce a regular file, is made read-only (0444) so a consumer's in-place write fails loudly instead of corrupting the shared copy, and is published with one atomic rename. A hit links out and touches the entry's last-use clock under a shared lock that eviction will hold exclusively, closing the hit-vs-evict window. Destinations must not exist and must be absolute paths on the cache's mount; cross-filesystem destinations fail with a dedicated error rather than a silent copy.

copyFile and its hole-preserving machinery lived in package main, usable only by atelet's own checkpoint staging. The filecache package is about to need the same copy (its copy-out mode hands consumers a private, hole-preserved copy of a cached artifact), so move the code where both can import it. Mechanical move, with one seam added: Copy(src, dst *os.File) exposes the engine on caller-owned handles, for callers that must open the source before its name can vanish or create the destination with O_EXCL. CopyFile keeps its os.Create semantics for the existing caller.

GetFileTo serves hits as read-only hard links, which is only safe for consumers that never write the staged file in place. GetFileCopyTo serves the same read-through cache as a private copy instead: the caller owns the resulting inode outright (mode 0600) and may mutate it freely, holes are preserved, and the destination may live on any filesystem. The copy reads a handle opened under the hit lock, so an eviction racing the copy retires only the entry's name — the bytes survive until the copy completes. Fetch dedup is unchanged: concurrent calls for one key share a single flight.

EvictUnused frees cache space least-recently-used first until a byte target is met, with a two-phase retire: inside the key's singleflight and the hit lock, a victim is re-verified (a moved last-use clock or an in-flight fetch vetoes) and renamed to a .rm-* dir, making it invisible to lookups; the slow physical deletion runs after all retires, outside the locks the hot path contends, so hits and fetches never wait on it. Entries younger than the store's min age are never touched, covering the window between publication and a consumer's first link. Entries whose data a consumer still hard-links may be retired but count as pending rather than freed bytes - the kernel returns that space when the last consumer link goes - so eviction can only ever cost a re-download, never break a consumer. FreedBytes is credited per entry only after its physical removal succeeds; a failed removal leaves the bytes in a .rm-* dir for the startup sweep and out of the freed count.

State the package's consumer-protection contracts in the package doc (link-out immunity, min-age sizing, the read-only shared-bytes rule, and key immutability), and pin them with a race-detector stress test: getters and evictors hammer the same keys concurrently, and every get must succeed with intact content - eviction may force refetches but can never fail a caller, corrupt a served file, or leave half-states in the store.

Eitan Yarmush (EItanya)

I'd be ok merging this as is, but honestly the claudisms in here make the code pretty hard to parse. Any chance we could do a run through to cleanup the comment language to be more readable, and get rid of some of the callback test seam misdirection?

Eitan Yarmush (EItanya) · 2026-09-10T11:20:39Z

+// createDestFile is a test seam for CopyFile's destination.
+var createDestFile = func(name string) (io.WriteCloser, error) { return os.Create(name) }


I personally hate this style of test seam callback, but that's definitely a nit. These agents love callbacks. Could we instead pass an interface into CopyFile if it needs this type of functionality?

Removed the seam entirely in the top commit (5a6b6ba) — CopyFile opens with os.Create, and the sparseDest interface went with it (copySparse now writes real *os.Files). The userspace fallback is tested through the public API instead: CopyFile to /dev/shm makes copy_file_range fail with EXDEV for real. The close-error test only existed because the seam made it possible; it's deleted with it.

Eitan Yarmush (EItanya) · 2026-09-10T11:23:16Z

+// contexts and bounded by the store's fetch timeout. Its error is delivered
+// to every caller waiting on the flight, wrapped with %w so error
+// classification (errors.Is) sees through the store.
+type FileFetcher func(ctx context.Context, dstPath string) error


Same nit here, I personally prefer interfaces but it's a personal style thing

Kept this one as a func type deliberately: it's not a test seam but the production injection point — call sites bind a GCS client and object URI via closure (func(ctx, dst) error { return ategcs.FetchLocalFileFromGCSWithZstd(ctx, client, uri, dst) }). An interface would force a wrapper struct at each call site for the same one method; func types are the stdlib idiom here (http.HandlerFunc, filepath.WalkFunc). Happy to revisit if you feel strongly.

Eitan Yarmush (EItanya) · 2026-09-10T11:24:47Z

+// getTo is the read-through loop shared by GetFileTo and GetFileCopyTo:
+// serve a hit via out (link or copy), else run the singleflight fetch and
+// retry.
+func (s *Store) getTo(ctx context.Context, key Key, dst string, fetch FileFetcher, out func(Key, string) (bool, error)) error {


This function takes 2 callbacks as test seams. I hate to focus on this, but I worry that it makes the code harder to read, and that the testing strategy is a bit myopic if it's testing each level like that.

Fair that it read that way, but neither parameter is a test seam: fetch is the caller's downloader (the public API), and the second is the one real difference between GetFileTo and GetFileCopyTo — serve the hit as a hard link or as a private copy — shared so the miss/fetch/retry loop isn't duplicated. The top commit (5a6b6ba) gives it a named, documented type (serveHit) so the loop reads as that strategy split. No test overrides either of them.

Eitan Yarmush (EItanya) · 2026-09-10T11:27:36Z

+//
+// A crash can leave debris in tmp/ (a fetch that never finished) or .rm-*
+// dirs (an eviction that renamed but never removed); SweepDebris reaps both
+// and runs once at startup, before the store serves requests. Everything


Do we also want some sort of schedule to clear these out?

Good catch — there was a real gap behind this. tmp/ must stay startup-swept only (a later sweep would delete in-flight fetches' working directories), but .rm-* dirs from a failed RemoveAll used to wait for the next restart. As of the top commit (5a6b6ba) every eviction pass retries those removals first (race-free under evictMu; only eviction creates them while the store serves), and SweepDebris's doc states both halves explicitly.

Eitan Yarmush (EItanya) · 2026-09-10T11:37:11Z

The file naming here is confusing, this makes it seem like the whole filecache is located here, but really the GC behavior is located here. Can we somehow separate it so it's more clear what functionality exists in this file

Split in the top commit (5a6b6ba): doc.go (package contract), store.go (store lifecycle), sweep.go (startup debris sweep), with the sizing helpers moved next to their consumer in evict.go. filecache.go is gone.

Remove the createDestFile test seam and the sparseDest indirection: CopyFile creates its destination directly, copySparse writes real files, and the userspace fallback is tested through the public API by copying across filesystems (copy_file_range fails with EXDEV on a /dev/shm destination). Name getTo's serving-mode parameter (serveHit) so the shared loop reads as the strategy split it is, not a test seam. Eviction passes now retry .rm-* leftovers a previous pass failed to remove, so those bytes no longer wait for the next restart's sweep; tmp/ stays startup-swept only, since a later sweep would delete in-flight fetches' working directories. Split filecache.go into doc.go, store.go, and sweep.go, moving the sizing helpers next to their consumer in evict.go, and shorten the package's comments.

Dmitry Berkovich (dberkov) · 2026-09-11T00:44:04Z

Cleanup pass is up as a new commit on this branch — 5a6b6ba, on top of the reviewed chain (no history rewrite, plain push): test seams removed with the userspace copy path now tested through the public API across real filesystems, getTo's serving-mode parameter named and documented, eviction passes retrying leftover .rm-* removals, filecache.go split by concern, and a general pass shortening the comment prose. Replied inline on each thread with specifics; the one thing kept as-is is FileFetcher as a func type (production injection point, stdlib idiom — reasoning on the thread).

This was referenced Sep 5, 2026

atelet: add filecache store skeleton #1512

Closed

atelet: add filecache singleflight retrieval (GetFileTo) #1513

Closed

atelet: add filecache eviction (EvictUnused) #1515

Closed

Dmitry Berkovich (dberkov) force-pushed the filecache-m1 branch 2 times, most recently from f7d72fb to ee7dd93 Compare September 5, 2026 04:59

Eitan Yarmush (EItanya) previously requested changes Sep 5, 2026

View reviewed changes

Eitan Yarmush (EItanya) reviewed Sep 5, 2026

View reviewed changes

Dmitry Berkovich (dberkov) force-pushed the filecache-m1 branch 2 times, most recently from a6e923e to 2c6a7d9 Compare September 9, 2026 00:59

Dmitry Berkovich (dberkov) mentioned this pull request Sep 9, 2026

atelet: serve golden snapshot restores from the node-local file cache #1551

Draft

Dmitry Berkovich (dberkov) force-pushed the filecache-m1 branch from 2c6a7d9 to 6121ecd Compare September 9, 2026 01:25

Eitan Yarmush (EItanya) reviewed Sep 9, 2026

View reviewed changes

Dmitry Berkovich (dberkov) added 6 commits September 9, 2026 21:01

Dmitry Berkovich (dberkov) force-pushed the filecache-m1 branch from 6121ecd to 5797a85 Compare September 10, 2026 04:04

Eitan Yarmush (EItanya) reviewed Sep 10, 2026

View reviewed changes

Dmitry Berkovich (dberkov) mentioned this pull request Sep 11, 2026

atelet: simplify the file cache internals per review #1605

Closed

Eitan Yarmush (EItanya) approved these changes Sep 11, 2026

View reviewed changes

Eitan Yarmush (EItanya) merged commit 3280b02 into agent-substrate:main Sep 11, 2026
18 of 19 checks passed

Dmitry Berkovich (dberkov) deleted the filecache-m1 branch September 11, 2026 22:15

		// createDestFile is a test seam for CopyFile's destination.
		var createDestFile = func(name string) (io.WriteCloser, error) { return os.Create(name) }

Uh oh!

Conversation

Dmitry Berkovich (dberkov) commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eitan Yarmush (EItanya) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) commented Sep 9, 2026

Uh oh!

Eitan Yarmush (EItanya) left a comment

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) commented Sep 9, 2026

Uh oh!

Eitan Yarmush (EItanya) left a comment

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitry Berkovich (dberkov) commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dmitry Berkovich (dberkov) commented Sep 5, 2026 •

edited

Loading

Eitan Yarmush (EItanya) left a comment •

edited

Loading

Eitan Yarmush (EItanya) left a comment •

edited

Loading

Dmitry Berkovich (dberkov) Sep 11, 2026 •

edited

Loading

Dmitry Berkovich (dberkov) Sep 11, 2026 •

edited

Loading

Dmitry Berkovich (dberkov) Sep 11, 2026 •

edited

Loading

Dmitry Berkovich (dberkov) Sep 11, 2026 •

edited

Loading

Dmitry Berkovich (dberkov) commented Sep 11, 2026 •

edited

Loading